Conversation
wxiaoguang
left a comment
There was a problem hiding this comment.
The tests written by AI are not right
I am not done still trying to understand that old bad code lol |
|
Suggest to do it step by step with a plan. Design the table for the future, but the first step should refactor the existing code first. |
ca66385 to
d035e0d
Compare
Co-authored-by: Claude (Opus 4.6) [email protected]
Co-authored-by: Claude (Opus 4.6) [email protected]
made it like you suggested with the unique |
| repoIDIndex.AddColumn("repo_id") | ||
| indices = append(indices, repoIDIndex) | ||
|
|
||
| statusIndex := schemas.NewIndex("idx_notification_status", schemas.IndexType) |
There was a problem hiding this comment.
Why this index removed. I think it's important.
7389b31 to
7e4e3c8
Compare
| UserID int64 | ||
| RepoID int64 | ||
| IssueID int64 | ||
| UniqueKey string |
There was a problem hiding this comment.
It's better to not expose the key outside of the model? So that we could keep it as before but use uniquekey when building conditions.
|
|
||
| if notificationExists(notifications, issue.ID, userID) { | ||
| existing, err := GetIssueNotification(ctx, userID, issue.ID) | ||
| if err != nil { |
There was a problem hiding this comment.
Don't return if err is ErrNotExist.
| statusIndex.AddColumn("status") | ||
| indices = append(indices, statusIndex) | ||
|
|
||
| sourceIndex := schemas.NewIndex("idx_notification_source", schemas.IndexType) |
There was a problem hiding this comment.
source removed from index but ToConds still use it as a search condition.
Restarts #34803
@lunny :
This PR improved notifications.